Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a hook for intercepting decoding errors #99

Closed
wants to merge 1 commit into from

Conversation

jpsim
Copy link

@jpsim jpsim commented Feb 4, 2024

Motivation

It's useful for clients to be able to have a centralized way to handle decoding errors, which can be useful for logging or telemetry.

See apple/swift-openapi-generator#522 for more discussion on this.

Modifications

Add a new DecodingErrorHandler protocol which has a single willThrow(_ error: any Error) requirement.

A decoding handler can be passed to the client configuration on initialization.

This is just a proof-of-concept, if this direction is reasonable, it should be fleshed out to handle all decoding paths.

Result

With this change, errors produced by body decoders can be intercepted in a centralized way for all methods going through the Client.

Test Plan

I added a simple unit test. More tests would need to be added to sufficiently cover the functionality being introduced here.

This is just a proof-of-concept, if this direction is reasonable, it
should be fleshed out to handle all decoding paths.

See apple/swift-openapi-generator#522 for
motivation.
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall, apologies for the delay on this PR, it somehow slipped through unnoticed.

@@ -96,6 +96,10 @@ extension JSONDecoder.DateDecodingStrategy {
}
}

public protocol DecodingErrorHandler: Sendable {
func willThrow(_ error: any Error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be interested in the context this was thrown in? See ClientError and ServerError, where we add more context.

@@ -113,9 +119,11 @@ public struct Configuration: Sendable {
/// - multipartBoundaryGenerator: The generator to use when creating mutlipart bodies.
public init(
dateTranscoder: any DateTranscoder = .iso8601,
multipartBoundaryGenerator: any MultipartBoundaryGenerator = .random
multipartBoundaryGenerator: any MultipartBoundaryGenerator = .random,
decodingErrorHandler: (any DecodingErrorHandler)? = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please just add a deprecated 2-parameter version of this initializer in Deprecated.swift, as adding a param is technically a source break. Similar to #102 (comment)

@czechboy0
Copy link
Contributor

Hi @jpsim - are you still interested in a solution here? I'm happy with either you picking this up again, or letting us take over. It'd just be good to finalize the details with a concrete use case in mind.

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as waiting on @jpsim for now

@jpsim
Copy link
Author

jpsim commented Jan 15, 2025

I won't be pursuing this further.

@jpsim jpsim closed this Jan 15, 2025
@czechboy0
Copy link
Contributor

I won't be pursuing this further.

May I ask if it's because you found a workflow where you don't need it, or if you don't have the bandwidth to drive the PR?

@daviduzumeri
Copy link

We're using this library pretty heavily and this would be a fantastic addition that would help us catch a lot of stuff in Datadog instead of finding out API responses aren't decoding properly from customer complaints.

@czechboy0
Copy link
Contributor

We're open to this enhancement, and with JP closing this PR, I think it'd be appropriate for someone else to open a new PR and we can discuss next steps there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants